Modernize codebase: housekeeping, DeleteCollection, and scope-hardened GetDocument#125
Merged
hsinatfootprintai merged 2 commits intomainfrom Apr 24, 2026
Merged
Conversation
Housekeeping changes with no API behavior impact. All edits produce the same wire-level responses and the same database state as before. Build & CI - Expand .gitignore to cover the restcol binary, coverage output, IDE directories, and env files. - Add a Makefile with build, test, test-race, test-full, vet, tidy, run-local, run-postgres, gen-proto, clean, and help targets. - Upgrade CI to Go 1.24.1 (matches go.mod toolchain), add explicit `go vet` and `go build` steps, and run tests with the race detector. Latent bugs surfaced by enabling `go vet` in CI - pkg/models/collections/schema.go: fix 7 copylocks violations by switching SwagValueValue methods to pointer receivers and using pointer conversions instead of dereferencing structpb.Value (which carries a sync.Mutex via protoimpl.MessageState). Also fix a malformed GORM struct tag on ModelFieldSchema.FieldName. - pkg/storage/documents/documents_test.go: add `testing.Short()` skip guards to TestDocumentCURD_Delete and TestDocumentCURD_Delete_WithWrongScope so CI's `-short` run does not try to connect to a non-existent DB. - Replace 13x `context.TODO()` with `context.Background()` in document storage tests. GORM indexes - Add `index` tags on ModelCollection.ModelProjectID, ModelSchema.ModelCollectionID, and a composite `docScope` index on ModelDocument (model_project_id, model_collection_id, created_at) to back ListByProjectID, GetLatestSchema, ModelDocument.Query, and the new Count/DeleteByCollection helpers. Migrations - Add a `migrations/` directory with a baseline SQL migration that adds the same three indexes explicitly for environments running with --restcol_auto_migrate=false (i.e. production). Includes a README documenting the workflow and the deferred work to fully wire golang-migrate. Docs & typos - Rewrite README.md with correct ports (50090 gRPC / 50091 HTTP), real curl examples for every endpoint, architecture diagram, repo layout, configuration flags, and an explicit warning that the shipped AnnonymousClaimParser + AllowEveryOne combo is dev-only. - Fix two typos: "foreigh" -> "foreign", "shema:" -> "schema:". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ction
Two changes intertwined: a pure structural refactor and the behavior
additions from Wave 5 of the modernization effort. Separated in the diff
but shipped together because they share files.
Structural refactor (no behavior change)
- Move production server code from integrationtest/server/server.go to
pkg/server/app/app.go; package integrationtestserver -> serverapp. The
old location confused production vs. integration-test boundaries.
- Rename pkg/dummy -> pkg/bootstrap; DummyModelProject -> DefaultModelProject,
NewDummyProject -> NewDefaultProject. Added a package doc explaining why
the default project exists (anonymous auth needs a tenant).
- Split pkg/app/app.go (536 lines) into app.go (struct, constructor,
shared helpers, swagger), collections.go (CRUD for collections), and
documents.go (CRUD + query for documents). Every exported method now
carries a doc comment.
- Update main.go, integrationtest/suite.go, integrationtest/setup_test.go
imports to match.
API changes
- Proto: add `bool force = 3` to DeleteCollectionRequest (api/restcol.proto).
Regenerated api/pb/* and api/openapiv2/*.swagger.json via `buf generate`.
- Implement DeleteCollection. Previously returned NotImpl. New behavior:
- Missing collection_id -> BadParameters
- Non-existent collection -> NotFound
- Non-empty collection without force -> StatusConflicted
- Non-empty collection with force=true -> cascade-deletes all documents
before soft-deleting the collection itself
- Harden GetDocument scope check. Storage.Get uses .Find(), which returns
an empty record + nil error when the (project, collection, document)
tuple does not match. The handler now converts an empty ID into a
NotFound error so a wrong project or collection ID cannot produce a
blank success response.
- Add input validation to GetDocument: empty collection_id or document_id
returns BadParameters; invalid document_id format likewise.
- Fix error-swallow bug in getProjectIdFromCtx: when ProjectInfor.GetProjectID
fails the error was logged and dropped. Now propagates.
Storage additions
- CollectionCURD.Delete(ctx, tableName, pid, cid) for the DeleteCollection
path.
- DocumentCURD.CountByCollection and DeleteByCollection to support the
reject-vs-cascade decision and the cascade operation itself.
Tests
- Add pkg/app/handlers_test.go with table-driven handler tests covering:
DeleteCollection validation, existence, empty-success, non-empty
reject-without-force (StatusConflicted), force-cascade (document gone
afterward); GetDocument validation matrix and the new cross-collection
scope-mismatch -> NotFound assertion; DeleteDocument validation;
GetCollection missing-id rejection. All tests are guarded by
testing.Short() so CI's -short path stays postgres-free.
- Add a fixedProjectResolver test double implementing
sdinsureruntime.ProjectResolver so handlers can be exercised without
the full gRPC middleware stack.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive modernization pass informed by a full-repo audit. Two commits:
chore: modernize build, CI, and repo hygiene— no API behavior change.refactor(app): relocate server, split handlers, implement DeleteCollection— structural reorg bundled with the newDeleteCollectionbehavior and the hardenedGetDocumentscope check.What's in the chore commit
.gitignoreexpanded (binary, coverage, IDE files,.env).Makefilewithbuild,test,test-race,test-full,vet,tidy,run-local,run-postgres,gen-proto,clean,help.go.modtoolchain); explicitgo vetandgo buildsteps;-raceenabled on tests.go vet:copylocksviolations inpkg/models/collections/schema.gowhereSwagValueValuewas copied by value, carrying async.Mutexviastructpb.Value. Fixed by switching to pointer receivers and pointer conversions.ModelFieldSchema.FieldName.TestDocumentCURD_Delete,TestDocumentCURD_Delete_WithWrongScope) missingtesting.Short()guards — would have broken CI's-shortrun.indextags added on frequently queried FK columns (ModelCollection.ModelProjectID,ModelSchema.ModelCollectionID) and a compositedocScopeindex onModelDocument(model_project_id, model_collection_id, created_at).migrations/scaffold with baseline SQL mirroring the new indexes for production (where--restcol_auto_migrate=false), plus a README explaining the workflow.curlexamples for every endpoint, architecture diagram, repo layout, config flags, and a dev-only-auth warning.context.TODO()→context.Background()in 13 test spots.What's in the refactor commit
Structural (no behavior change):
integrationtest/server/server.go→pkg/server/app/app.go— production server no longer lives underintegrationtest/.pkg/dummy/→pkg/bootstrap/withDummyModelProject→DefaultModelProject.pkg/app/app.go(536 lines) split into topical files:app.go(114),collections.go(152),documents.go(317). Every exported method got a doc comment.API changes:
bool force = 3toDeleteCollectionRequest. Regeneratedapi/pb/*andapi/openapiv2/*.swagger.json.DeleteCollectionimplemented (wasNotImpl):collection_id→BadParametersNotFoundforce→StatusConflicted(message guides caller to passforce=true)force=true→ cascade soft-deletes documents firstGetDocumentscope hardening: storage uses.Find()which returns(emptyRecord, nil)on scope mismatch; handler now converts empty ID intoNotFoundto prevent blank success responses under the wrong project/collection. Also added input validation for empty/malformed IDs.getProjectIdFromCtx: previously logged and swallowedGetProjectID()errors; now propagates.Storage additions for
DeleteCollection:CollectionCURD.DeleteDocumentCURD.CountByCollectionDocumentCURD.DeleteByCollectionTests:
pkg/app/handlers_test.gowith table-driven coverage of the new behavior and afixedProjectResolvertest double. Guarded bytesting.Short()so CI stays postgres-free.Deliberately deferred
golang-migrateswap at server startup.AutoMigrateis already opt-in via--restcol_auto_migrate(defaults to off in production), so the urgency is low; the full swap needs a baseline-matching migration and staged rollout. Migration files + README lay the groundwork.pkg/runtime/js) — product-level decision on timeout/memory policy.github.com/sdinsure/agent— black-box dep; out of scope.Test plan
go vet ./...go build ./...go test -race ./... -shortmake run-postgres && make test-full(requires a clean local postgres)curlexamples in the new README🤖 Generated with Claude Code